fix(pi): narrow converter guards — trailing assistant, tool/image/surrogate, signed-thinking#32
Open
iceteaSA wants to merge 2 commits into
Open
fix(pi): narrow converter guards — trailing assistant, tool/image/surrogate, signed-thinking#32iceteaSA wants to merge 2 commits into
iceteaSA wants to merge 2 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
iceteaSA
force-pushed
the
fix/pi-improvements
branch
from
b659ab4 to
768e897
Compare
There was a problem hiding this comment.
No issues found across 3 files
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
iceteaSA
force-pushed
the
fix/pi-improvements
branch
from
768e897 to
432f588
Compare
Contributor
|
Reviewed this in context with OpenCode PR anomalyco/opencode#30182 and the related OpenCode-side healing PRs here (#49, #51). My read:
Recommendation: split this PR. Keep the tests and the narrow Pi conversion guards, including trailing assistant stripping. Drop or separately rework the historical signed-thinking downgrade into a much narrower fix backed by a concrete Pi repro. |
iceteaSA
force-pushed
the
fix/pi-improvements
branch
2 times, most recently
from
c3d0d62 to
0003da7
Compare
iceteaSA
changed the title
Contributor
Author
Dropped it |
…r guards - Strip trailing assistant-role messages from converted requests to prevent Anthropic prefill 400 errors on some models - Add Unicode flag to surrogate regex for spec compliance - Simplify sanitizeToolId with cleaner length check - Improve error content guard to catch empty string case - Add comprehensive test coverage for all pi conversion logic
Signed thinking blocks must be sent back byte-identical — the signature is computed over the original text, so sanitizing would alter it and Anthropic rejects the block as modified. Send signed thinking verbatim instead of sanitizing it. The one exception is a signed block containing a lone (unpaired) UTF-16 surrogate: the signature cannot survive sanitization and the raw surrogate is invalid UTF-8 (400). Detect lone surrogates with hasLoneSurrogate() and, only in that concrete case, drop the signature and downgrade to sanitized text. No broad historical-thinking healing — valid signed/redacted thinking is preserved. (Signed-thinking reordering is handled OpenCode-side in #30182.)
iceteaSA
force-pushed
the
fix/pi-improvements
branch
from
0003da7 to
ae0626e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Narrow, Pi-owned converter hardening for
packages/pi/src/convert.ts, beyond what was merged in #17. All changes are scoped to concrete invalid shapes the Pi converter can produce — no broad payload "healing".Commit 1: strip trailing assistant messages, improve sanitize and error guards
/guflag) for spec-compliant surrogate-pair replacement.sanitizeToolIdwith a cleaner length check (removes a redundant guard).content === '') that upstream misses.Commit 2: preserve signed thinking verbatim, guard lone surrogates
hasLoneSurrogate()detects exactly that case, and only then is the signature dropped and the text sanitized.Scope note (per review)
This PR intentionally does not include the broad "downgrade all historical signed thinking to text" healing. That approach can avoid 400s but discards valid signed/redacted reasoning when the request shape is otherwise correct. The signed-thinking reordering problem is better fixed at the OpenCode layer in anomalyco/opencode#30182 (preventing the Anthropic reorder from displacing signed/redacted reasoning in the first place). For Pi, the converter should avoid broad destructive healing unless a concrete Pi-generated invalid shape requires it — none currently does, so it is omitted here.